New design for wpaccessibility.org #220
Conversation
|
…18-new-design # Conflicts: # docs/topics/forms/fieldsets.md
rianrietveld
changed the title
joedolson
left a comment
joedolson
left a comment
There was a problem hiding this comment.
I have some minor comments, but on the whole I think we should just merge this. From a practical standpoint, this is very hard to review - combining a refactor of the docs JS with other changes is something I wouldn't generally do, as it makes the diff much harder to parse.
There are some minor things - like I think the vertical height of the secondary nav items is too tall, and I don't fully agree with the accessible name of the primary logo with the img inside the link, but I think they can be adjusted as follow ups.
The description is in the index and shown in results, but isn't being searched; that'll probably need to be tweaked, as well.
| @@ -1,23 +1,22 @@ | |||
| <div class="search"> | |||
| <form class="search-input-wrap" role="search" action="{{ site.baseurl }}/search/" method="get"> | |||
| <search class="search"> | |||
There was a problem hiding this comment.
Do you think this has sufficient compatibility? The search element has only been supported on Safari since version 17, so older Macs & iOS devices may not get the landmark role from the element. Other browsers are on similar timelines, but Safari is unique in that devices that can't be updated to more recent OS also can't update their browsers.
There was a problem hiding this comment.
I did this because Axe complained that the search functionality was not in a landmark and I thought <search> was a good option.
I will change it into <section aria-label="Search"> and add the role search back on the form element
_includes/components/sidebar.html
Outdated
| <img src="{{site.baseurl}}/assets/images/logo.jpg" alt="Accessibility: Advocate Equal Access"> | ||
| <a href="{{ '/' | relative_url }}" class="site-title lh-tight"> | ||
| {{ site.title}}</a> | ||
| <img src="{{site.baseurl}}/assets/images/logo-wa11ypuu.png" alt="WP Accessibility logo, to homepage"> |
There was a problem hiding this comment.
I don't personally like having the accessible name of the link have all this information in it. This becomes "WP Accessibility logo, to home page WP Accessibility Knowledge Base", which seems excessive to me.
There was a problem hiding this comment.
This discussion 🤯 ...
I was trying to avoid an aria-label.
I will add the a11y name as an aria-label and make it "WP Accessibility logo, to the homepage".
_sass/layout/_navigation.scss
Outdated
|
|
||
| .nav-list-item { | ||
| position: relative; | ||
| min-height: 48px; |
There was a problem hiding this comment.
I think this gives too much height on secondary nav items. I'd remove it and use something like padding: 4px 0;
There was a problem hiding this comment.
Oh, I added the min-height to the wrong selector. Jeffrey told he wanted to have all menu items 48px high, to comply with WCAG SC 2.5.5. Now all heights are the same.
|
@joedolson thanks for the review 🙏 |
The related issue number: #218 (new design) and #221 (description to search)
Design Jeffrey Lauwers:
https://www.figma.com/design/MTMLnjkAGJvYrZjTeu5ert/WP-Accessibility-Knowledge-Base?node-id=33-1237&t=byeUhJr81gGKjPNv-0
Preview: https://wpaccessibility.org/pr-preview/pr-220.
Note:
The search on mobile is not the best experience ux-wise.
We need to discuss if the drop down on mobile is useful and if we can move the search on. mobile above the menu.